Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move percy keys from encrypted .travis.yml entries to travis config #9314

Merged
merged 2 commits into from
May 17, 2017
Merged

Move percy keys from encrypted .travis.yml entries to travis config #9314

merged 2 commits into from
May 17, 2017

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented May 12, 2017

Something is up with how Travis consumes encrypted keys from .travis.yml. The tests are now using the keys as set in the travis settings page. See https://travis-ci.org/ampproject/amphtml/builds/231611667 for an example of a successful percy run for a push to master.

Issues #8, #8340, #9069

@rsimha rsimha requested a review from dreamofabear May 12, 2017 19:42
@rsimha rsimha added this to the Sprint H1 May milestone May 12, 2017
@rsimha rsimha self-assigned this May 12, 2017
@adelinamart adelinamart removed this from the Sprint H1 May milestone May 12, 2017
@rsimha rsimha requested a review from erwinmombay May 15, 2017 17:45
@rsimha
Copy link
Contributor Author

rsimha commented May 16, 2017

@erwinmombay: Ping.

@rsimha rsimha requested a review from kmh287 May 17, 2017 14:45
@rsimha
Copy link
Contributor Author

rsimha commented May 17, 2017

@kmh287 for review.

@@ -58,7 +58,9 @@ function extractPercyKeys() {
'Error: PERCY_TOKEN doesn\'t look like a valid Percy API key'));
process.exit(1);
}
util.log('Percy token: ', util.colors.magenta('<redacted>'));
// Note: The secret percy token only gets printed during local use. Travis
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who will be able to see the token?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only engineers who want to do a local visual diff run, who already have access to the tokens. Those who access the public travis logs will not see the token in the logs.

@@ -39,8 +39,6 @@ branches:
env:
global:
- NPM_CONFIG_PROGRESS="false"
- secure: nvLqQdU2CGwvsQ3E1M/6HgBJAfmSzd90bDYQ1vkAkwGj+VOVQauqj7RDJACJS2XysJjzIvzNUOtKFvTSzThye167sKAZnQMmj9U8ATdlJfv5xq88QkkoCuJIOPtVnOWS8gBrDAy9d+Xwi22ZcE5kAAov1gNDmnX6YcPwW8y09vD+Jb1WkxofwTR5U3dP7pxVNRwgXY12g2lmbj+TZy5RHvjs1/ESSVtt/5Wq+Ubv9O6KAVPrh/VdVTYyvgynvwzz5jG3qe7981W8rUNIt+ihzdxJMSaoQkiBs5UM3qi8ibqKI2KHdDmGQg+vc/c5mjCCd9FAz7lF+fJJQ3YThTLmIN3u7Hil+o92OaAt2knwtZ3HRv/jbVQfnU0L9hqHw/kBYE37O4x4/ToJyR3EgbvFXqgiVph0eDUnsmi8PZc5FUiwb8r85thQmfUB+urkOXbnOyHQ9B74T+UjAvlaqLdQ53/9UTzgvLNm2z6RU65G9QYJVMSzj82K2rZ9AyVwjZMp/iuQDoUcUsuMhKDh2genHnxRtQa/TbLQM05oYHRqFa9oT3Q7C7QSSL8mmi/QMfGlTfxhM49CZsbcU2qbMCMCL7ZGWaSiXiEy7XICOzJnOiKFowDK5ihAZaQpRNquMwUSIMnnqrFgfWDbQSUmkOAwSm+2u2+SsXhT+WQfraRfSSI=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were moved elsewhere right? Is it okay that these were publicly visible on github?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is travis' mechanism to safely do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are now part of the travis config, and are in a mode where they are not printed within logs. This is exactly how we store our sauce tokens.

@rsimha rsimha merged commit 79b29b4 into ampproject:master May 17, 2017
@rsimha rsimha deleted the 2017-05-12-PercyUnencryptedToken branch May 17, 2017 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants